fix: sync node.imgs for legacy context menu in Vue Nodes mode#8143
fix: sync node.imgs for legacy context menu in Vue Nodes mode#8143christian-byrne merged 4 commits intomainfrom
Conversation
Add syncLegacyNodeImgs store method to sync loaded image elements to node.imgs for backwards compatibility with legacy systems (Copy Image, Open Image, Save Image, Open in Mask Editor). - Only runs when vueNodesMode is enabled - Reuses already-loaded img element from Vue component (no duplicate loading) - Store owns the sync logic, component just hands off the element - Simplify mask editor handling to call composable directly Fixes missing context menu options on SaveImage vue node. Amp-Thread-ID: https://ampcode.com/threads/T-019bba3e-0ad8-754a-bd50-5cf17165d5a6 Co-authored-by: Amp <amp@ampcode.com>
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughReplaces command-based mask editor opening with a new Changes
Sequence DiagramsequenceDiagram
participant ImagePreview as ImagePreview.vue
participant Handler as handleImageLoad / handleEditMask
participant Store as nodeOutputStore
participant Graph as LiteGraph.rootGraph
participant Mask as useMaskEditor
rect rgba(200,150,255,0.5)
Note over ImagePreview,Graph: Image load → legacy node sync
ImagePreview->>Handler: image loaded (element, nodeId, activeIndex)
Handler->>Store: syncLegacyNodeImgs(nodeId, element, activeIndex)
alt LiteGraph.vueNodesMode == true
Store->>Graph: getNodeById(nodeId)
Graph-->>Store: node (or null)
opt node found
Store->>Store: node.imgs = [element]\nnode.imageIndex = activeIndex
end
end
end
rect rgba(100,200,255,0.5)
Note over ImagePreview,Mask: Open mask editor flow
ImagePreview->>Handler: user clicks "Edit Mask" (with nodeId)
Handler->>Mask: openMaskEditor(node)
Mask->>Mask: initialize editor with node
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 02/18/2026, 11:29:58 PM UTC 🔗 Links🎉 Your Storybook is ready for review! |
|
Playwright: ❌ 518 passed, 2 failed · 4 flaky ❌ Failed Tests📊 Browser Reports
|
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 21.7 kB (baseline 21.7 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 905 kB (baseline 905 kB) • 🟢 -205 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 69 kB (baseline 69 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 9 added / 9 removed Panels & Settings — 427 kB (baseline 427 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 10 added / 10 removed User & Accounts — 16.1 kB (baseline 16.1 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 5 added / 5 removed Editors & Dialogs — 785 B (baseline 785 B) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 1 added / 1 removed UI Components — 36.6 kB (baseline 36.6 kB) • ⚪ 0 BReusable component library chunks
Status: 5 added / 5 removed Data & Services — 2.16 MB (baseline 2.16 MB) • 🔴 +267 BStores, services, APIs, and repositories
Status: 13 added / 13 removed Utilities & Hooks — 237 kB (baseline 237 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 13 added / 13 removed Vendor & Third-Party — 8.69 MB (baseline 8.69 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 7.42 MB (baseline 7.42 MB) • ⚪ 0 BBundles that do not match a named category
Status: 51 added / 51 removed |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/stores/imagePreviewStore.test.ts (1)
26-31: Prefer Partial for mock typing.The current cast hides incompleteness; using
Partial<LGraphNode>makes the intent explicit and aligns with test typing conventions.♻️ Suggested adjustment
-const createMockNode = (overrides: Record<string, unknown> = {}): LGraphNode => - ({ - id: 1, - type: 'TestNode', - ...overrides - }) as LGraphNode +const createMockNode = (overrides: Partial<LGraphNode> = {}): LGraphNode => + ({ + id: 1, + type: 'TestNode', + ...overrides + }) as Partial<LGraphNode> as LGraphNodeBased on learnings, prefer
Partial<Interface>casts for incomplete mock objects.
🤖 Fix all issues with AI agents
In `@src/renderer/extensions/vueNodes/components/ImagePreview.vue`:
- Around line 214-218: Hoist the call to the composable out of the event handler
by invoking useMaskEditor() once in the component setup and storing its return
(e.g. const maskEditor = useMaskEditor()) so handleEditMask uses
maskEditor.openMaskEditor(node) instead of calling useMaskEditor() inside the
handler; update the setup block to create maskEditor and ensure handleEditMask
references that variable to avoid missing component instance/injection timing
issues.
## Summary Fix for COM-14110: Preview image does not display new outputs in vue-nodes. ## Problem The merge logic in `setOutputsByLocatorId` updated `app.nodeOutputs` but returned early without updating the reactive `nodeOutputs.value` ref. This caused Vue components to never receive merged output updates because only the non-reactive `app.nodeOutputs` was being updated. ## Solution Added `nodeOutputs.value[nodeLocatorId] = existingOutput` after the merge loop, before the return statement. ## Testing - Added 2 unit tests covering the merge behavior - All 4076 existing unit tests pass - Typechecks pass - Lint passes ## Notes - Related open PRs touching same files: #8143, #8366 - potential minor conflicts possible Fixes COM-14110 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8479-fix-update-reactive-ref-after-merge-in-imagePreviewStore-2f86d73d365081f1a145fa5a9782515f) by [Unito](https://www.unito.io) Co-authored-by: Amp <amp@ampcode.com>
## Summary Fix for COM-14110: Preview image does not display new outputs in vue-nodes. ## Problem The merge logic in `setOutputsByLocatorId` updated `app.nodeOutputs` but returned early without updating the reactive `nodeOutputs.value` ref. This caused Vue components to never receive merged output updates because only the non-reactive `app.nodeOutputs` was being updated. ## Solution Added `nodeOutputs.value[nodeLocatorId] = existingOutput` after the merge loop, before the return statement. ## Testing - Added 2 unit tests covering the merge behavior - All 4076 existing unit tests pass - Typechecks pass - Lint passes ## Notes - Related open PRs touching same files: #8143, #8366 - potential minor conflicts possible Fixes COM-14110 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8479-fix-update-reactive-ref-after-merge-in-imagePreviewStore-2f86d73d365081f1a145fa5a9782515f) by [Unito](https://www.unito.io) Co-authored-by: Amp <amp@ampcode.com>
## Summary Fix for COM-14110: Preview image does not display new outputs in vue-nodes. ## Problem The merge logic in `setOutputsByLocatorId` updated `app.nodeOutputs` but returned early without updating the reactive `nodeOutputs.value` ref. This caused Vue components to never receive merged output updates because only the non-reactive `app.nodeOutputs` was being updated. ## Solution Added `nodeOutputs.value[nodeLocatorId] = existingOutput` after the merge loop, before the return statement. ## Testing - Added 2 unit tests covering the merge behavior - All 4076 existing unit tests pass - Typechecks pass - Lint passes ## Notes - Related open PRs touching same files: #8143, #8366 - potential minor conflicts possible Fixes COM-14110 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8479-fix-update-reactive-ref-after-merge-in-imagePreviewStore-2f86d73d365081f1a145fa5a9782515f) by [Unito](https://www.unito.io) Co-authored-by: Amp <amp@ampcode.com>
## Summary Fix for COM-14110: Preview image does not display new outputs in vue-nodes. ## Problem The merge logic in `setOutputsByLocatorId` updated `app.nodeOutputs` but returned early without updating the reactive `nodeOutputs.value` ref. This caused Vue components to never receive merged output updates because only the non-reactive `app.nodeOutputs` was being updated. ## Solution Added `nodeOutputs.value[nodeLocatorId] = existingOutput` after the merge loop, before the return statement. ## Testing - Added 2 unit tests covering the merge behavior - All 4076 existing unit tests pass - Typechecks pass - Lint passes ## Notes - Related open PRs touching same files: #8143, #8366 - potential minor conflicts possible Fixes COM-14110 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8479-fix-update-reactive-ref-after-merge-in-imagePreviewStore-2f86d73d365081f1a145fa5a9782515f) by [Unito](https://www.unito.io) Co-authored-by: Amp <amp@ampcode.com>
| } | ||
|
|
||
| if (props.nodeId) { | ||
| nodeOutputStore.syncLegacyNodeImgs(props.nodeId, img, currentIndex.value) |
There was a problem hiding this comment.
Seeing sync always worries me.
There was a problem hiding this comment.
It's just doing what happens in litegraph
There was a problem hiding this comment.
Why does that not comfort me?
| ) { | ||
| if (!LiteGraph.vueNodesMode) return | ||
|
|
||
| const node = app.rootGraph?.getNodeById(Number(nodeId)) |
There was a problem hiding this comment.
We really need to standardize our NodeId type.
DrJKL
left a comment
There was a problem hiding this comment.
I still think this is dirty, but we can clean it up later.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
browser_tests/tests/vueNodes/interactions/node/imagePreview.spec.ts (1)
24-27: Prefer centralized TestIds for stable selectors.The
.image-preview,.mask-editor-dialog, and.p-contextmenuclass selectors are brittle. Please use the shared TestIds frombrowser_tests/fixtures/selectors.ts(or add them) for these containers, while keeping text-based assertions for menu items.As per coding guidelines: Use the centralized TestIds from
browser_tests/fixtures/selectors.tsfor test element selection.Also applies to: 38-38, 48-49
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@browser_tests/tests/vueNodes/interactions/node/imagePreview.spec.ts` around lines 24 - 27, Replace brittle class-based locators ('.image-preview', '.mask-editor-dialog', '.p-contextmenu') in imagePreview.spec.ts with the centralized TestIds exported from browser_tests/fixtures/selectors.ts: import and use the appropriate TestId constants (e.g., IMAGE_PREVIEW, MASK_EDITOR_DIALOG, CONTEXT_MENU) when constructing locators for comfyPage.page.locator(...) or locator(...). Keep menu-item assertions that rely on visible text unchanged. Update all occurrences mentioned (around the current checks and the other spots noted) to use these TestId constants so selectors are stable and consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@browser_tests/tests/vueNodes/interactions/node/imagePreview.spec.ts`:
- Around line 12-16: The helper loadImageOnNode currently derives the fixture
type from test.info() which is invalid; change its parameter type to the
concrete imported ComfyPage type (import ComfyPage from the fixture module) and
update the function signature to accept comfyPage: ComfyPage. Also replace
class-based DOM selectors ('.image-preview', '.mask-editor-dialog',
'.p-contextmenu') with centralized TestIds (e.g., TestIds.ImagePreview,
TestIds.MaskEditorDialog, TestIds.ContextMenu) by adding those keys to
browser_tests/fixtures/selectors.ts if missing and updating the Vue node
components to expose data-testid attributes that match those TestIds so the spec
uses getByTestId / locator(`[data-testid="${TestIds...}"]`).
---
Nitpick comments:
In `@browser_tests/tests/vueNodes/interactions/node/imagePreview.spec.ts`:
- Around line 24-27: Replace brittle class-based locators ('.image-preview',
'.mask-editor-dialog', '.p-contextmenu') in imagePreview.spec.ts with the
centralized TestIds exported from browser_tests/fixtures/selectors.ts: import
and use the appropriate TestId constants (e.g., IMAGE_PREVIEW,
MASK_EDITOR_DIALOG, CONTEXT_MENU) when constructing locators for
comfyPage.page.locator(...) or locator(...). Keep menu-item assertions that rely
on visible text unchanged. Update all occurrences mentioned (around the current
checks and the other spots noted) to use these TestId constants so selectors are
stable and consistent.
| async function loadImageOnNode( | ||
| comfyPage: Awaited< | ||
| ReturnType<(typeof test)['info']> | ||
| >['fixtures']['comfyPage'] | ||
| ) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and inspect the ComfyPage fixture module
echo "=== ComfyPage fixture file ==="
fd -a "ComfyPage.ts" browser_tests/fixtures -x head -100 {}
echo -e "\n=== Export patterns in ComfyPage ==="
fd -a "ComfyPage.ts" browser_tests/fixtures -x rg "export (class|type|interface|const|function|default)" {} -nRepository: Comfy-Org/ComfyUI_frontend
Length of output: 3648
🏁 Script executed:
#!/bin/bash
# Inspect the imagePreview.spec.ts file around the flagged lines
echo "=== imagePreview.spec.ts (lines 1-30) ==="
head -50 browser_tests/tests/vueNodes/interactions/node/imagePreview.spec.ts
echo -e "\n=== Check fixture import and usage ==="
rg -n "import.*comfyPage|loadImageOnNode|test\.info" browser_tests/tests/vueNodes/interactions/node/imagePreview.spec.ts -A 3 -B 1Repository: Comfy-Org/ComfyUI_frontend
Length of output: 2765
🏁 Script executed:
#!/bin/bash
# Check if Playwright TestInfo exposes fixtures through test.info()
# by looking at how fixtures are typically accessed in test files
echo "=== Playwright fixture usage patterns in test files ==="
rg -r "test\.info\(\)" browser_tests/tests -t ts --max-count=5 -B 2 -A 2Repository: Comfy-Org/ComfyUI_frontend
Length of output: 984
🏁 Script executed:
#!/bin/bash
# Verify TestIds exports for image preview, mask editor, and context menu
echo "=== TestIds defined in selectors.ts ==="
rg -n "image|mask|context" browser_tests/fixtures/selectors.ts -i -B 1 -A 1Repository: Comfy-Org/ComfyUI_frontend
Length of output: 234
🏁 Script executed:
#!/bin/bash
# Check if there are TestIds for image preview or mask editor elsewhere
echo "=== All TestIds entries ==="
head -100 browser_tests/fixtures/selectors.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 2356
🏁 Script executed:
#!/bin/bash
# Check if image preview, mask editor, context menu components have test IDs in source
echo "=== Search for image-preview and mask-editor-dialog in src ==="
rg "image-preview|mask-editor-dialog" src --include="*.vue" --include="*.ts" -l
echo -e "\n=== Check context menu component ==="
rg "p-contextmenu|ContextMenu" src --include="*.vue" --include="*.ts" -l | head -5Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1071
🏁 Script executed:
#!/bin/bash
# Check if image preview, mask editor, context menu components have test IDs in source
echo "=== Search for image-preview in src ==="
rg "image-preview" src -t vue -t ts
echo -e "\n=== Search for mask-editor-dialog ==="
rg "mask-editor-dialog" src -t vue -t ts
echo -e "\n=== Search for p-contextmenu ==="
rg "p-contextmenu" src -t vue -t tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 283
🏁 Script executed:
#!/bin/bash
# Check if image preview, mask editor, context menu components have test IDs in source
echo "=== Search for image-preview in src ==="
rg "image-preview" src
echo -e "\n=== Search for mask-editor-dialog ==="
rg "mask-editor-dialog" src
echo -e "\n=== Search for p-contextmenu ==="
rg "p-contextmenu" srcRepository: Comfy-Org/ComfyUI_frontend
Length of output: 1163
🏁 Script executed:
#!/bin/bash
# Verify what's accessible on the comfyPageFixture object
echo "=== comfyPageFixture definition ==="
rg -n "comfyPageFixture = base.extend" browser_tests/fixtures/ComfyPage.ts -A 20Repository: Comfy-Org/ComfyUI_frontend
Length of output: 974
Fixture type derivation is invalid; use the imported ComfyPage type directly.
Playwright TestInfo (from test.info()) doesn't expose a fixtures property, making the type path inaccessible. Import and use the concrete ComfyPage type instead.
Additionally, use centralized TestIds for element selection instead of class-based selectors (.image-preview, .mask-editor-dialog, .p-contextmenu). If TestIds doesn't have entries for these Vue node components yet, add them to browser_tests/fixtures/selectors.ts and update the components to use data-testid attributes.
✅ Proposed fix for fixture type
import { expect } from '@playwright/test'
import { comfyPageFixture as test } from '../../../../fixtures/ComfyPage'
+import type { ComfyPage } from '../../../../fixtures/ComfyPage'
async function loadImageOnNode(
- comfyPage: Awaited<
- ReturnType<(typeof test)['info']>
- >['fixtures']['comfyPage']
+ comfyPage: ComfyPage
) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@browser_tests/tests/vueNodes/interactions/node/imagePreview.spec.ts` around
lines 12 - 16, The helper loadImageOnNode currently derives the fixture type
from test.info() which is invalid; change its parameter type to the concrete
imported ComfyPage type (import ComfyPage from the fixture module) and update
the function signature to accept comfyPage: ComfyPage. Also replace class-based
DOM selectors ('.image-preview', '.mask-editor-dialog', '.p-contextmenu') with
centralized TestIds (e.g., TestIds.ImagePreview, TestIds.MaskEditorDialog,
TestIds.ContextMenu) by adding those keys to browser_tests/fixtures/selectors.ts
if missing and updating the Vue node components to expose data-testid attributes
that match those TestIds so the spec uses getByTestId /
locator(`[data-testid="${TestIds...}"]`).
- Fix createPinia() → createTestingPinia() (merge conflict artifact) - Fix 'should return empty string if output is animated' test to mock isAnimatedOutput instead of setting node property - Hoist useMaskEditor() to component setup (CodeRabbit review) - Use as unknown as LGraphNode for clearer mock typing Amp-Thread-ID: https://ampcode.com/threads/T-019c730f-b5f6-713f-87c2-4f39d4eb6cf5
## Summary Fixes missing "Copy Image", "Open Image", "Save Image", and "Open in Mask Editor" context menu options on SaveImage nodes when Vue Nodes mode is enabled. ## Changes - Add `syncLegacyNodeImgs` store method to sync loaded image elements to `node.imgs` - Call sync on image load in ImagePreview component - Simplify mask editor handling to call composable directly ## Technical Details - Only runs when `vueNodesMode` is enabled (no impact on legacy mode) - Reuses already-loaded `<img>` element from Vue (no duplicate network requests) - Store owns the sync logic, component just hands off the element Supersedes #7416 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8143-fix-sync-node-imgs-for-legacy-context-menu-in-Vue-Nodes-mode-2ec6d73d365081c59d42cd1722779b61) by [Unito](https://www.unito.io) --------- Co-authored-by: Amp <amp@ampcode.com>
## Summary Fixes missing "Copy Image", "Open Image", "Save Image", and "Open in Mask Editor" context menu options on SaveImage nodes when Vue Nodes mode is enabled. ## Changes - Add `syncLegacyNodeImgs` store method to sync loaded image elements to `node.imgs` - Call sync on image load in ImagePreview component - Simplify mask editor handling to call composable directly ## Technical Details - Only runs when `vueNodesMode` is enabled (no impact on legacy mode) - Reuses already-loaded `<img>` element from Vue (no duplicate network requests) - Store owns the sync logic, component just hands off the element Supersedes #7416 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8143-fix-sync-node-imgs-for-legacy-context-menu-in-Vue-Nodes-mode-2ec6d73d365081c59d42cd1722779b61) by [Unito](https://www.unito.io) --------- Co-authored-by: Amp <amp@ampcode.com>
|
@christian-byrne Successfully backported to #8969 |
|
@christian-byrne Successfully backported to #8970 |
The tests from PR #8143 were written against stale ComfyPage APIs that were refactored in PR #8510: - comfyPage.dragAndDropFile → comfyPage.dragDrop.dragAndDropFile - comfyPage.setSetting → comfyPage.settings.setSetting - comfyPage.loadWorkflow → comfyPage.workflow.loadWorkflow - comfyPage.getNodeRefsByType → comfyPage.nodeOps.getNodeRefsByType - comfyPage type param → import ComfyPage type directly Also removes test.fixme since the root cause was API mismatch. Amp-Thread-ID: https://ampcode.com/threads/T-019c73c1-be32-7687-b758-672fedaf61af
The tests from PR #8143 were written against stale ComfyPage APIs that were refactored in PR #8510: - comfyPage.dragAndDropFile → comfyPage.dragDrop.dragAndDropFile - comfyPage.setSetting → comfyPage.settings.setSetting - comfyPage.loadWorkflow → comfyPage.workflow.loadWorkflow - comfyPage.getNodeRefsByType → comfyPage.nodeOps.getNodeRefsByType - comfyPage type param → import ComfyPage type directly Also removes test.fixme since the root cause was API mismatch. Amp-Thread-ID: https://ampcode.com/threads/T-019c73c1-be32-7687-b758-672fedaf61af
The tests from PR #8143 were written against stale ComfyPage APIs that were refactored in PR #8510: - comfyPage.dragAndDropFile → comfyPage.dragDrop.dragAndDropFile - comfyPage.setSetting → comfyPage.settings.setSetting - comfyPage.loadWorkflow → comfyPage.workflow.loadWorkflow - comfyPage.getNodeRefsByType → comfyPage.nodeOps.getNodeRefsByType - comfyPage type param → import ComfyPage type directly Also removes test.fixme since the root cause was API mismatch. Amp-Thread-ID: https://ampcode.com/threads/T-019c73c1-be32-7687-b758-672fedaf61af
Summary
Fixes missing "Copy Image", "Open Image", "Save Image", and "Open in Mask Editor" context menu options on SaveImage nodes when Vue Nodes mode is enabled.
Changes
syncLegacyNodeImgsstore method to sync loaded image elements tonode.imgsTechnical Details
vueNodesModeis enabled (no impact on legacy mode)<img>element from Vue (no duplicate network requests)Supersedes #7416
┆Issue is synchronized with this Notion page by Unito